Adding Unit Tests For SSL Operations #1845

vikashprajapati-cell · 2023-09-15T10:01:25Z

Description

SSL (Secure Sockets Layer) is a cryptographic protocol used to establish secure connections between clients and servers. Testing SSL functionality ensures the secure transmission of data between clients and servers. Adding tests for SSL involves evaluating how the Babelfish codebase handles SSL connections.
We are keen on conducting thorough unit testing of the SslHandshake process that occurs during the prelogin stage. Our unit testing approach involves meticulously examining each step of the handshake, simulating various scenarios, and verifying the correct implementation of the SSL handshake. Through this comprehensive testing, we aim to identify any potential vulnerabilities that may exist.

Note-
To execute SSL-related tests, it is essential to enable the SSL flag during the build process using the --with-openssl command. This ensures that the necessary components for SSL testing are included. In the event that the SSL flag is not enabled, only non-SSL tests will be executed.

Authored-by: Vikash Prajapati [email protected]
Signed-off-by: Vikash Prajapati [email protected]

Test Scenarios Covered

jdbc_testdb=# CREATE EXTENSION IF NOT EXISTS babelfishpg_unit;
CREATE EXTENSION
jdbc_testdb=# select * from babelfishpg_unit.babelfishpg_unit_run_tests();
                  test_name                  | status |                   message                   | runtime | enabled 
---------------------------------------------+--------+---------------------------------------------+---------+---------
 GreaterThanOrEqualToCheck_INT4_FIXEDDECIMAL | pass   |                                             |       6 | enabled
 LesserThanOrEqualToCheck_INT4_FIXEDDECIMAL  | pass   |                                             |       1 | enabled
 NotEqualToCheck_INT4_FIXEDDECIMAL           | pass   |                                             |       1 | enabled
 EqualToCheck_INT4_FIXEDDECIMAL              | pass   |                                             |       2 | enabled
 LesserThanCheck_INT4_FIXEDDECIMAL           | pass   |                                             |       1 | enabled
 Comparison_INT4_FIXEDDECIMAL                | pass   |                                             |       3 | enabled
 FIXEDDECIMALUM                              | pass   | fixeddecimal out of range                   |      11 | enabled
 GreaterThanOrEqualToCheck_FIXEDDECIMAL_INT2 | pass   |                                             |       1 | enabled
 LesserThanOrEqualToCheck_FIXEDDECIMAL_INT2  | pass   |                                             |       1 | enabled
 GreaterThanCheck_FIXEDDECIMAL_INT2          | pass   |                                             |       1 | enabled
 NotEqualToCheck_FIXEDDECIMAL_INT2           | pass   |                                             |       1 | enabled
 Comparison_FIXEDDECIMAL_INT2                | pass   |                                             |       2 | enabled
 Testing_SSL_HANDSHAKE_READ                  | pass   |                                             |      21 | enabled
 TestingOverSize_SSL_HANDSHAKE_READ          | pass   |  SSL packet expand more than one TDS packet |      52 | enabled
 TestingPacketType_SSL_HANDSHAKE_READ        | pass   |                                             |       6 | enabled
 Testing_SSL_HANDSHAKE_WRITE                 | pass   |                                             |       4 | enabled
 TestingSizeCheck_SSL_HANDSHAKE_WRITE        | pass   |  There is nothing to write                  |       1 | enabled
(17 rows)

Check List

Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is under the terms of the Apache 2.0 and PostgreSQL licenses, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Vikash Prajapati <[email protected]>

contrib/babelfishpg_unit/test_ssl_read.c

contrib/babelfishpg_tds/src/backend/tds/tdssecure.c

contrib/babelfishpg_unit/test_ssl_write.c

contrib/babelfishpg_unit/test_ssl_read.c

Signed-off-by: Vikash Prajapati <[email protected]>

thephantomthief

We should be running these new tests as part of github actions. Currently I think that is not happening

Signed-off-by: Vikash Prajapati <[email protected]>

surendravishnoi · 2023-09-25T11:42:12Z

contrib/babelfishpg_unit/test_ssl_read.c

+
+    obtained = test_ssl_handshake_read(h, buf, expected, mock_socket_read, ReadPointer);
+
+    size_buf = strlen(buf);


strlen only works in string is null terminated

Updated in the latest commit.

surendravishnoi · 2023-09-25T11:51:03Z

contrib/babelfishpg_unit/test_ssl_read.c

+     */
+    if(expected == obtained)
+    {
+        TEST_ASSERT_TESTCASE(*((unsigned char*)expected_str) == *((unsigned char*)buf), testResult);


why are we only comparing first character?

Updated in the latest commit with complete string comparision.

surendravishnoi · 2023-09-25T13:58:27Z

contrib/babelfishpg_unit/test_ssl_read.c

+    snprintf(expected_str, MAX_TEST_MESSAGE_LENGTH, "%d", expected);
+    snprintf(obtained_str, MAX_TEST_MESSAGE_LENGTH, "%d", obtained);
+
+    TEST_ASSERT_TESTCASE(expected != obtained, testResult);


this is not clear, what do we expect here that multi packet message should be handled properly, if yes why results should not match

Updated the logic in the latest commit.

surendravishnoi · 2023-09-25T14:46:28Z

contrib/babelfishpg_tds/src/backend/tds/tdssecure.c

+     */
+
+    int res;
+    tds_secure_raw_write = mock_socket_write;


should we reset it after the unit testing?

Yes, have updated in the next revision.

surendravishnoi · 2023-09-25T14:47:16Z

contrib/babelfishpg_tds/src/backend/tds/tdssecure.c

+
+    int res;
+    tds_secure_raw_write = mock_socket_write;
+    unit_testing = true;


what if there is an error during testing and unit_testing remains true? I think not an issue in current set of tests but could be a problem later

Handled this using PG_TRY() and PG_FINALLY() block in the latest commit.

…gresql/babelfish_extensions into SSL_UNIT_TESTS

Signed-off-by: Vikash Prajapati <[email protected]>

…gresql/babelfish_extensions into SSL_UNIT_TESTS

Signed-off-by: Vikash Prajapati <[email protected]>

surendravishnoi · 2023-10-03T05:46:23Z

contrib/babelfishpg_tds/src/backend/tds/tdssecure.c

 					(errcode(ERRCODE_ADMIN_SHUTDOWN),
 					 errmsg("terminating connection due to unexpected ssl packet header")));
-
+			


unnecessary white spaces

surendravishnoi · 2023-10-03T05:46:43Z

contrib/babelfishpg_tds/src/backend/tds/tdssecure.c

+    tds_secure_raw_read = mock_socket_read;
+    unit_testing = true;
+    pkt_bytes_read = ReadPointer;
+	PG_TRY();


indentation

surendravishnoi · 2023-10-03T05:47:04Z

contrib/babelfishpg_tds/src/backend/tds/tdssecure.c

+    }
+    PG_END_TRY();
+
+	tds_secure_raw_read = secure_raw_read;


put this also in finally block

surendravishnoi · 2023-10-03T05:49:27Z

contrib/babelfishpg_tds/src/backend/tds/tdssecure.c

+    tds_secure_raw_write = mock_socket_write;
+    unit_testing = true;
+
+	PG_TRY();


indentation, also put write API reset in finally block

surendravishnoi · 2023-10-03T06:22:55Z

contrib/babelfishpg_unit/test_ssl_read.c

+
+    obtained = test_ssl_handshake_read(h, buf, expected, mock_socket_read, ReadPointer);
+
+    null_terminated_expected = strdup(expected_str);


why these copies? we can compare expected_str and buf directly. However, is buf guaranteed to be null terminated?

surendravishnoi · 2023-10-03T06:35:37Z

contrib/babelfishpg_unit/test_ssl_read.c

+    memcpy(obtained_str, buf, obtained);
+    obtained_next = test_ssl_handshake_read(h, buf, expected - obtained, mock_socket_read, ReadPointer);
+    memcpy(obtained_str + obtained, buf, obtained_next);
+    null_terminated_expected = strdup(expected_str);


again why this copy? also if obtained_str is not null terminated how will strdup will work. You just need to put at '\0' at the end

surendravishnoi · 2023-10-03T06:36:47Z

contrib/babelfishpg_unit/test_ssl_read.c

+    TestResult* testResult = palloc0(sizeof(TestResult));
+    testResult->result = true;
+
+    prelogin_request = strdup("1201000F0000010011A25E4571");


can you explain the message format? how does it force two packet reads?

surendravishnoi · 2023-10-03T07:38:18Z

contrib/babelfishpg_unit/test_ssl_write.c

+
+    h = BIO_new(BIO_s_mem());
+
+    prelogin = malloc(strlen(buf)/2 + 8);


why are we writing buf/2 rather than buf?

surendravishnoi · 2023-10-03T08:47:46Z

contrib/babelfishpg_unit/test_ssl_write.c

+    snprintf(obtained_str, MAX_TEST_MESSAGE_LENGTH, "%d", obtained);
+
+    TEST_ASSERT_TESTCASE(expected == obtained, testResult);
+    if(testResult->result == true)


why both if/else cases?

surendravishnoi · 2023-10-03T08:48:13Z

contrib/babelfishpg_unit/test_ssl_write.c

+    expected = -1;
+    obtained = test_ssl_handshake_write(h, buf, expected, mock_socket_write);
+
+    snprintf(expected_str, MAX_TEST_MESSAGE_LENGTH, "%d", expected);


these are not used anywhere?

…gresql/babelfish_extensions into SSL_UNIT_TESTS

Signed-off-by: Vikash Prajapati <[email protected]>

surendravishnoi · 2023-10-09T12:01:49Z

contrib/babelfishpg_tds/src/backend/tds/tdssecure.c

+	}
+	PG_END_TRY();
+
+		return res;


indentation

surendravishnoi · 2023-10-09T13:28:28Z

contrib/babelfishpg_unit/test_ssl_read.c

+    TestResult* testResult = palloc0(sizeof(TestResult));
+    testResult->result = true;
+
+    prelogin_request = strdup("1201000B00000100115461A23E");


again, why are we manipulating length here?

surendravishnoi · 2023-10-09T13:34:15Z

contrib/babelfishpg_unit/test_ssl_write.c

+
+    obtained_str = (char *)malloc(obtained + 1);
+    strncpy(obtained_str, buf, obtained);
+    obtained_str[obtained] = '\0';


what are we achieving with this comparison? both expected and obtained strings are copy of buf only. We should compared expected against prelogin string where mock socket write happens (after skipping header part)

Adding Unit Tests For SSL Operations

b8fcd86

Signed-off-by: Vikash Prajapati <[email protected]>

vikashprajapati-cell requested review from ahmed-shameem and surendravishnoi September 15, 2023 12:35

ahmed-shameem suggested changes Sep 18, 2023

View reviewed changes

Addressing comments

f803d0a

Signed-off-by: Vikash Prajapati <[email protected]>

vikashprajapati-cell requested a review from ahmed-shameem September 20, 2023 05:57

ahmed-shameem approved these changes Sep 20, 2023

View reviewed changes

vikashprajapati-cell requested a review from thephantomthief September 25, 2023 10:38

thephantomthief reviewed Sep 25, 2023

View reviewed changes

Address comment

2e20339

Signed-off-by: Vikash Prajapati <[email protected]>

surendravishnoi reviewed Sep 25, 2023

View reviewed changes

Vikash Prajapati added 3 commits September 25, 2023 15:42

Merge branch 'BABEL_3_X_DEV' into SSL_UNIT_TESTS

9ea9173

Merge branch 'BABEL_3_X_DEV' of https://github.com/babelfish-for-post…

e2a341c

…gresql/babelfish_extensions into SSL_UNIT_TESTS

Address comments

78292a5

Signed-off-by: Vikash Prajapati <[email protected]>

surendravishnoi previously approved these changes Sep 26, 2023

View reviewed changes

Vikash Prajapati added 4 commits September 27, 2023 09:09

Merge branch 'BABEL_3_X_DEV' of https://github.com/babelfish-for-post…

3363c91

…gresql/babelfish_extensions into SSL_UNIT_TESTS

Merge branch 'BABEL_3_X_DEV' of https://github.com/babelfish-for-post…

a80c59a

…gresql/babelfish_extensions into SSL_UNIT_TESTS

Merge branch 'BABEL_3_X_DEV' of https://github.com/babelfish-for-post…

efcef39

…gresql/babelfish_extensions into SSL_UNIT_TESTS

Address comments

1e8bc4a

Signed-off-by: Vikash Prajapati <[email protected]>

vikashprajapati-cell dismissed surendravishnoi’s stale review via 1e8bc4a October 3, 2023 04:50

surendravishnoi reviewed Oct 3, 2023

View reviewed changes

Vikash Prajapati added 7 commits October 9, 2023 04:16

Merge branch 'BABEL_3_X_DEV' of https://github.com/babelfish-for-post…

ea0cfe1

…gresql/babelfish_extensions into SSL_UNIT_TESTS

Merge branch 'BABEL_3_X_DEV' of https://github.com/babelfish-for-post…

98e91dc

…gresql/babelfish_extensions into SSL_UNIT_TESTS

Address comments

14790fd

Signed-off-by: Vikash Prajapati <[email protected]>

Minor changes

f603714

Signed-off-by: Vikash Prajapati <[email protected]>

Indentation

a3aa700

Signed-off-by: Vikash Prajapati <[email protected]>

Indentation

a032b44

Signed-off-by: Vikash Prajapati <[email protected]>

Indentation

81c51a5

Signed-off-by: Vikash Prajapati <[email protected]>

surendravishnoi reviewed Oct 9, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Unit Tests For SSL Operations #1845

Adding Unit Tests For SSL Operations #1845

vikashprajapati-cell commented Sep 15, 2023 •

edited

Loading

thephantomthief left a comment

surendravishnoi Sep 25, 2023

vikashprajapati-cell Oct 3, 2023

surendravishnoi Sep 25, 2023

vikashprajapati-cell Oct 3, 2023

surendravishnoi Sep 25, 2023

vikashprajapati-cell Oct 3, 2023

surendravishnoi Sep 25, 2023

vikashprajapati-cell Oct 3, 2023

surendravishnoi Sep 25, 2023

vikashprajapati-cell Oct 3, 2023 •

edited

Loading

surendravishnoi Oct 3, 2023

surendravishnoi Oct 3, 2023

surendravishnoi Oct 3, 2023

surendravishnoi Oct 3, 2023

surendravishnoi Oct 3, 2023

surendravishnoi Oct 3, 2023

surendravishnoi Oct 3, 2023

surendravishnoi Oct 3, 2023

surendravishnoi Oct 3, 2023

surendravishnoi Oct 3, 2023

surendravishnoi Oct 9, 2023

surendravishnoi Oct 9, 2023

surendravishnoi Oct 9, 2023


		obtained = test_ssl_handshake_read(h, buf, expected, mock_socket_read, ReadPointer);

		size_buf = strlen(buf);

		(errcode(ERRCODE_ADMIN_SHUTDOWN),
		errmsg("terminating connection due to unexpected ssl packet header")));


		obtained = test_ssl_handshake_read(h, buf, expected, mock_socket_read, ReadPointer);

		null_terminated_expected = strdup(expected_str);


		h = BIO_new(BIO_s_mem());

		prelogin = malloc(strlen(buf)/2 + 8);

Adding Unit Tests For SSL Operations #1845

Are you sure you want to change the base?

Adding Unit Tests For SSL Operations #1845

Conversation

vikashprajapati-cell commented Sep 15, 2023 • edited Loading

Description

Test Scenarios Covered

Check List

thephantomthief left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikashprajapati-cell Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikashprajapati-cell commented Sep 15, 2023 •

edited

Loading

vikashprajapati-cell Oct 3, 2023 •

edited

Loading